feat: support `concat` for strings #2604

comphead · 2025-10-18T17:36:30Z

Which issue does this PR close?

Closes #2552 .

Rationale for this change

What changes are included in this PR?

How are these changes tested?

comphead · 2025-10-18T17:40:32Z

Depends on #2586

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala

codecov-commenter · 2025-10-26T04:09:03Z

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.22%. Comparing base (f09f8af) to head (164dbfe).
⚠️ Report is 649 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/main/scala/org/apache/comet/serde/strings.scala	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2604      +/-   ##
============================================
+ Coverage     56.12%   59.22%   +3.10%     
- Complexity      976     1448     +472     
============================================
  Files           119      147      +28     
  Lines         11743    13762    +2019     
  Branches       2251     2365     +114     
============================================
+ Hits           6591     8151    +1560     
- Misses         4012     4387     +375     
- Partials       1140     1224      +84

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

comphead · 2025-10-27T19:32:41Z

@andygrove please take another look.
concat works with strings, and for other datatypes it is being fixed in DataFusion apache/datafusion#18020

andygrove · 2025-10-28T19:15:03Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

    classOf[BitLength] -> CometScalarFunction("bit_length"),
    classOf[Chr] -> CometScalarFunction("char"),
    classOf[ConcatWs] -> CometScalarFunction("concat_ws"),
+    classOf[Concat] -> CometScalarFunction("concat"),


I think we need type checks so that we fall back to Spark for unsupported argument types?

Perhaps something like this?

object CometConcat extends CometScalarFunction[Concat]("concat") { override def getSupportLevel(expr: Concat): SupportLevel = { if (expr.children.forall(_.dataType == DataTypes.StringType)) { Compatible() } else { Incompatible(Some("Only string arguments are supported")) } } }

andygrove · 2025-10-28T19:16:01Z

fuzz-testing/src/main/scala/org/apache/comet/fuzz/Meta.scala

+    createFunctionWithInputTypes(
+      "concat",
+      Seq(SparkStringType, SparkStringType)
+    ), // TODO: variadic


I know that this PR is just to support string inputs in Comet concat, but the fuzz tester should ideally test for all types that Spark supports

andygrove · 2025-10-28T19:18:41Z

...urces/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q5.native_iceberg_compat/explain.txt

+         +- CometHashAggregate (67)
+            +- CometExpand (66)
+               +- CometUnion (65)
+                  :- CometHashAggregate (22)


Why is this hash aggregate now supported in Comet? I don't see concat used in the query.

https://github.com/apache/datafusion-benchmarks/blob/main/tpcds/queries-spark/q5.sql

andygrove · 2025-10-28T19:20:11Z

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala

+  // https://github.com/apache/datafusion-comet/issues/2647
+  ignore("test concat function - arrays") {


Could you enable these tests and use the recently added checkSparkAnswerAndFallbackReason method to make sure we are correctly falling back to Spark?

comphead · 2025-10-30T15:14:42Z

@andygrove PTAL

andygrove

LGTM. Thanks @comphead!

andygrove · 2025-10-30T16:41:38Z

In case anyone is wondering why more operators are now running natively, even though the TPC-DS queries do not contain concat ... Spark adds concats in aggregates and Comet was previously falling back to Spark:

:- HashAggregate [COMET: concat is not supported, Unsupported result expressions found in: Vector(MakeDecimal(sum(UnscaledValue(sales_price#3596))#3818L,17,2) AS sales#3606, MakeDecimal(sum(UnscaledValue(return_amt#3598))#3820L,17,2) AS returns#3608, (MakeDecimal(sum(UnscaledValue(profit#3597))#3819L,17,2) - MakeDecimal(sum(UnscaledValue(net_loss#3599))#3821L,17,2)) AS profit#3584, store channel AS channel#3840, concat(store, s_store_id#3856) AS id#3841)]

rluvaton · 2025-11-04T22:25:43Z

In case anyone is wondering why more operators are now running natively, even though the TPC-DS queries do not contain concat ... Spark adds concats in aggregates and Comet was previously falling back to Spark:
:- HashAggregate [COMET: concat is not supported, Unsupported result expressions found in: Vector(MakeDecimal(sum(UnscaledValue(sales_price#3596))#3818L,17,2) AS sales#3606, MakeDecimal(sum(UnscaledValue(return_amt#3598))#3820L,17,2) AS returns#3608, (MakeDecimal(sum(UnscaledValue(profit#3597))#3819L,17,2) - MakeDecimal(sum(UnscaledValue(net_loss#3599))#3821L,17,2)) AS profit#3584, store channel AS channel#3840, concat(store, s_store_id#3856) AS id#3841)]

concat exists in the query, no?:

'store' || s_store_id as id

https://github.com/apache/datafusion-benchmarks/blob/c472383a2e0570c85766cd8ec946614dd4fda542/tpcds/queries-spark/q5.sql#L105

but spark just move it to aggregate avoid copying more data perhaps?

comphead · 2025-11-04T22:54:00Z

:) tadaam @rluvaton thats' true, I totally forgot about || 🤦

comphead mentioned this pull request Oct 21, 2025

feat: Define function signatures in CometFuzz #2614

Merged

andygrove reviewed Oct 21, 2025

View reviewed changes

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala Show resolved Hide resolved

comphead mentioned this pull request Oct 24, 2025

feat: support concat with arrays and binary #2647

Open

comphead force-pushed the dev2 branch from eaa4506 to b43b1c2 Compare October 26, 2025 02:18

comphead changed the title ~~feat: support concat~~ feat: support concat for strings Oct 26, 2025

comphead requested a review from andygrove October 27, 2025 19:31

comphead marked this pull request as ready for review October 27, 2025 20:51

andygrove reviewed Oct 28, 2025

View reviewed changes

comphead mentioned this pull request Oct 29, 2025

Unsupported expressions found in spark sql unit test #2314

Open

19 tasks

comphead added 8 commits October 29, 2025 18:00

feat: support concat

433edb2

feat: support concat for strings

85a9c87

feat: support concat for strings

1c4215a

feat: support concat for strings

e0ca43e

feat: support concat for strings

08d95c6

feat: support concat for strings

73d2a01

feat: support concat for strings

a9ca651

feat: support concat for strings

164dbfe

comphead force-pushed the dev2 branch from 39c9220 to 164dbfe Compare October 30, 2025 01:00

andygrove approved these changes Oct 30, 2025

View reviewed changes

andygrove merged commit f826b65 into apache:main Oct 30, 2025
102 checks passed

		// https://github.com/apache/datafusion-comet/issues/2647
		ignore("test concat function - arrays") {

feat: support concat for strings #2604

feat: support concat for strings #2604

Uh oh!

Conversation

comphead commented Oct 18, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

comphead commented Oct 18, 2025

Uh oh!

Uh oh!

codecov-commenter commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

comphead commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andygrove Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

comphead commented Oct 30, 2025

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Oct 30, 2025

Uh oh!

Uh oh!

rluvaton commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comphead commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: support `concat` for strings #2604

feat: support `concat` for strings #2604

codecov-commenter commented Oct 26, 2025 •

edited

Loading

comphead commented Oct 27, 2025 •

edited

Loading

rluvaton commented Nov 4, 2025 •

edited

Loading